Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update TreeAdapter to work with const inputs #1775

Merged
merged 3 commits into from
Sep 18, 2024
Merged

Update TreeAdapter to work with const inputs #1775

merged 3 commits into from
Sep 18, 2024

Conversation

danrbailey
Copy link
Contributor

The TreeAdapter has a few problems, most notably related to the use of the tree()/constTree() methods. It appears that the TreeAdapter is intended to work with const inputs (because some of the typedefs remove const-ness) so it seems counter-intuitive that these methods do not always work for const inputs. In addition, the resulting error is confusing because the compiler complains about duplicate methods being defined.

I created a bunch of unit tests to validate against what I considered acceptable input and modified the TreeAdapter to fix these issues.

The one area where I extended it was to explicitly add support for TreeAdapter<const Grid<TreeT>>. This is very useful when const-ness is baked into an input template argument and seemed consistent with supporting TreeAdapter<const TreeT>.

All of the rest of the unit tests pass and none of these changes are expected to affect compatibility.

(@kmuseth)

@danrbailey
Update TreeAdapter to work correctly with all const arguments and add…
5862e8d 
… extensive unit tests

Signed-off-by: Dan Bailey <danbailey@ilm.com>
@danrbailey
Use non-const Grid in const version of type trait
6b9a6c2 
Signed-off-by: Dan Bailey <danbailey@ilm.com>
@danrbailey danrbailey mentioned this pull request Mar 14, 2024
Draft
@Idclip
Copy link
Contributor

Idclip commented Mar 18, 2024

What happens with an adapter templated on a const ValueAccessor? Or rather, is that behaviour still the same before and after this change

@danrbailey
Copy link
Contributor Author

What happens with an adapter templated on a const ValueAccessor? Or rather, is that behaviour still the same before and after this change

The behavior before is that it fails to compile, the behavior after is that it compiles successfully. I attached a unit test with these fixes, running the unit test without the fixes results in compilation errors on these lines - 544, 546, 550, 573, 575, 577, 589, 594, 595, 604, 605, 608, 610, 611, 612, 614.

Here's one example related to your question:

FloatGrid floatGrid;
TreeAdapter<tree::ValueAccessor<const FloatTree>>::tree(floatGrid);

I would expect this to work, but it results in this compile error:

openvdb/unittest/TestGrid.cc:604:38: error: no matching function for call to 'openvdb::v11_0::TreeAdapter<openvdb::v11_0::tree::ValueAccessorImpl<const openvdb::v11_0::tree::Tree<openvdb::v11_0::tree::RootNode<openvdb::v11_0::tree::InternalNode<openvdb::v11_0::tree::InternalNode<openvdb::v11_0::tree::LeafNode<float, 3>, 4>, 5> > >, true, void, openvdb::v11_0::index_sequence<0, 1, 2> > >::tree(openvdb::v11_0::FloatGrid&)'
  604 |         AdapterConstT::tree(floatGrid);
      |                                      ^
In file included from openvdb/openvdb.h:13,
                 from unittest/TestGrid.cc:5:
openvdb/Grid.h:1135:22: note: candidate: 'static openvdb::v11_0::TreeAdapter<openvdb::v11_0::tree::ValueAccessorImpl<_TreeType, true, void, typename std::decay<decltype (make_index_sequence_impl<(std::max((unsigned int)(1), _TreeType::DEPTH) - 1)>())>::type> >::TreeType& openvdb::v11_0::TreeAdapter<openvdb::v11_0::tree::ValueAccessorImpl<_TreeType, true, void, typename std::decay<decltype (make_index_sequence_impl<(std::max((unsigned int)(1), _TreeType::DEPTH) - 1)>())>::type> >::tree(openvdb::v11_0::TreeAdapter<openvdb::v11_0::tree::ValueAccessorImpl<_TreeType, true, void, typename std::decay<decltype (make_index_sequence_impl<(std::max((unsigned int)(1), _TreeType::DEPTH) - 1)>())>::type> >::TreeType&) [with _TreeType = const openvdb::v11_0::tree::Tree<openvdb::v11_0::tree::RootNode<openvdb::v11_0::tree::InternalNode<openvdb::v11_0::tree::InternalNode<openvdb::v11_0::tree::LeafNode<float, 3>, 4>, 5> > >; openvdb::v11_0::TreeAdapter<openvdb::v11_0::tree::ValueAccessorImpl<_TreeType, true, void, typename std::decay<decltype (make_index_sequence_impl<(std::max((unsigned int)(1), _TreeType::DEPTH) - 1)>())>::type> >::TreeType = const openvdb::v11_0::tree::Tree<openvdb::v11_0::tree::RootNode<openvdb::v11_0::tree::InternalNode<openvdb::v11_0::tree::InternalNode<openvdb::v11_0::tree::LeafNode<float, 3>, 4>, 5> > >]'
 1135 |     static TreeType& tree(TreeType& t) { return t; }
openvdb/Grid.h:1135:37: note:   no known conversion for argument 1 from 'openvdb::v11_0::FloatGrid' {aka 'openvdb::v11_0::Grid<openvdb::v11_0::tree::Tree<openvdb::v11_0::tree::RootNode<openvdb::v11_0::tree::InternalNode<openvdb::v11_0::tree::InternalNode<openvdb::v11_0::tree::LeafNode<float, 3>, 4>, 5> > > >'} to 'openvdb::v11_0::TreeAdapter<openvdb::v11_0::tree::ValueAccessorImpl<const openvdb::v11_0::tree::Tree<openvdb::v11_0::tree::RootNode<openvdb::v11_0::tree::InternalNode<openvdb::v11_0::tree::InternalNode<openvdb::v11_0::tree::LeafNode<float, 3>, 4>, 5> > >, true, void, openvdb::v11_0::index_sequence<0, 1, 2> > >::TreeType&' {aka 'const openvdb::v11_0::tree::Tree<openvdb::v11_0::tree::RootNode<openvdb::v11_0::tree::InternalNode<openvdb::v11_0::tree::InternalNode<openvdb::v11_0::tree::LeafNode<float, 3>, 4>, 5> > >&'}
 1135 |     static TreeType& tree(TreeType& t) { return t; }

To extract the problematic portion of the partial specialization:

template<typename _TreeType>
struct TreeAdapter<tree::ValueAccessor<_TreeType> >
{
    using TreeType             = _TreeType;
    ....
    using GridType             = Grid<TreeType>;
    ....

    ....
    static TreeType& tree(GridType& g) { return g.tree(); }
    ....
}

You can see that calling this with TreeAdapter<tree::ValueAccessor<const FloatTree>> results in tree(Grid<const FloatTree>) but no tree(Grid<FloatTree>&).

This is not expected to change behavior for any method which does instantiate correctly, just fill in all the missing instantiations.

@danrbailey danrbailey merged commit 3b473dd into AcademySoftwareFoundation:master Sep 18, 2024
41 checks passed
@danrbailey danrbailey deleted the tree_adapter_improvements branch September 18, 2024 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmlait jmlait jmlait approved these changes

@kmuseth kmuseth Awaiting requested review from kmuseth kmuseth is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@danrbailey @Idclip @jmlait